-
Notifications
You must be signed in to change notification settings - Fork 6.9k
fix: address external_directory gaps and improve symlink checks #7515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Based on the search results, I found one potentially related PR: Related PR:
This PR appears to address a similar security concern around symlink escapes in path containment checks, which is directly relevant to the current PR's focus on preventing symlink escape attacks. The current PR (7515) builds upon or may supersede this work by introducing the |
|
This approach makes sense for catching accidental escapes, but it doesn't account for symlinks that are intentionally placed in a project: With this change, accessing any of these would trigger a permission prompt every time, even though they're deliberately there. Could this be configurable instead? Something like: {
"permission": {
"follow_symlinks": "allow",
"external_directory": {
"~/shared-libs/*": "allow",
"*": "ask"
}
}
}Or if a symlink's resolved target already matches an That way users who want strict behavior can keep it, while others can explicitly permit symlinks they've set up intentionally. |
|
@rmk40 yeah that's a good point, let me ask Dax |
- Add Filesystem.containsResolved() that resolves symlinks before checking path containment, preventing symlink escape attacks - Add external_directory permission check to write.ts (was a TODO) - Update File.read/File.list to use containsResolved for existing files - Add dual-layer protection: lexical check first, then resolved check for existing paths - Document TOCTOU limitation in function docstring - Improve test robustness: explicit skip for unsupported symlink operations - Add comprehensive tests for symlink traversal scenarios
Unit tests for Filesystem.contains() and containsResolved() are in test/util/filesystem.test.ts - no need to duplicate in path-traversal.test.ts
ReadTool was missing the containsResolved() check that WriteTool and File.read() have. This meant a symlink inside the project pointing outside could bypass external_directory permission when using ReadTool.
…ypass - Add containsResolved() check to EditTool and PatchTool to prevent symlink escape attacks - Add bypassCwdCheck flag to write.ts for consistency with read.ts - Fix broken symlink bypass: check symlinks BEFORE exists() check so broken symlinks pointing outside are caught - Reorder read.ts to check symlinks before exists() for consistency
- Add 4 symlink tests to ReadTool: escape, broken escape, internal symlink - Create write.test.ts with 7 tests including 4 symlink tests - Tests verify external_directory permission is requested for symlink escapes - Tests verify broken symlinks pointing outside are caught
b5c59a5 to
c2cc7ac
Compare
|
Hm Dax says that symlinks to files outside cwd are okay |
|
updated this. @rekram1-node @thdxr - I don't entirely agree with the premise that you should (by default) follow symlinks outside of the current working dir. If your tools (e.g. bash) can be coerced (or discover themselves) into creating a symlink to an external directory outside of current state - IMO 1 + 2 are at odds with each other here as allowing 1 is just a different version of 2.
|
|
maybe i'm not following but we know users have a this would cause a prompt on those right? |
|
@thdxr yes, but what's the alternative? how you discern between explicit user intent (this want to be spammed by permissions prompts as little as the next guy, but I don't get how OpenCode can determine whether following do those cases exist because the permissions model was all or nothing prior to the object syntax in permissions? |
Addresses gaps in the
external_directorypermission checks where symlinks inside a project could escape to read/write files outside the project boundary.Filesystem.containsResolved()that resolves symlinks before checking path containment, preventing symlink escape attacks where a link likeproject/escape -> /etc/passwdwould bypass lexical checksexternal_directorypermission check toWriteTool(was a TODO) andReadToolFile.read()andFile.list()to use dual-layer protection: fast lexical check first, then resolved check for existing filescontainsResolved()- acceptable for the threat model of protecting against malicious symlinks in user-controlled directoriesTest coverage:
containsResolved()covering symlink chains, broken symlinks, relative symlink escapes, and positive cases for internal symlinksFile.read()blocks symlink escapes while allowing valid internal symlinksValidated in practice — I had OpenCode build itself and then test via the actual filesystem as well:
resulted in: